[#2039] Support Default Value Semantis: use default values reading AVRO files by shenodaguirguis · Pull Request #72 · linkedin/iceberg

shenodaguirguis · 2021-05-15T07:30:27Z

Last change of 3 (see description: apache/iceberg#2496 (comment))
The main changes here are in BuildAvroProjection.java and PruneColumns.java which makes sure that default value is copied over and used while reading projected columns with default values.
Other changes are utils, and testing changes.

There will be a followup PR to handle default values in serialization/deserializaiton, but this can go on parallel with ORC and Parquet changes.

[Update: during integration testing, I had to implement the default value serialization/deserialization. It works now on spark shell to select missing columns with default values and return the default value:

// required with non-null default
scala> spark.sql(s"select * from u_sguirgui.testi6").show
+---------+-----------------+                                                    
|firstname |     lastname          |
+---------+-----------------+
|        f        |               l.             |
|     Adam  | default lastname | <-- omitting the required col altogether
+---------+-----------------+
// optional with default value
scala> DaliSpark.createDataFrame("dalids:///u_sguirgui.test5", Map(DaliSpark.PROJECT_COLS -> "firstname, lastname")).show
+---------+----------------+
| firstname|        lastname      |
+---------+----------------+
|     Abel     |            Adam       |
|     Adam   |            null           | <-- using the null option
|     Adam|default lastname   |  <-- omitting the col altogether
+---------+----------------+

Also, during integration testing, it turns out that Avro default values for records are Maps, not Lists, so had to change that in the NestedField class.

shenodaguirguis · 2021-05-17T15:04:55Z

@wmoustafa @funcheetah @HotSushi please take a look

core/src/main/java/org/apache/iceberg/avro/AvroSchemaUtil.java

core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java

wmoustafa · 2021-05-23T07:08:29Z

core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java

+        Schema.Field newField = new Schema.Field(newFieldName, newFiledSchema, null, defaultValue);
+        newField.addProp(AvroSchemaUtil.FIELD_ID_PROP, expectedField.fieldId());


This branch seems to have common code with a the out if branch. Do you see room to combine and simplify?

we have 4 cases (for 2 conditions), for three of them we create a new field and add to updatedFileds, but different fields. The only thing that can be done is to create a shallow 3-lines function with 5 argument. I prefer to keep as is

wmoustafa · 2021-05-23T07:11:32Z

core/src/main/java/org/apache/iceberg/avro/PruneColumns.java

    Schema newSchemaReordered;
-    // if the newSchema is an optional schema, make sure the NULL option is always the first
-    if (isOptionSchemaWithNonNullFirstOption(newSchema)) {
+    // if the newSchema is an optional schema with no, or null, default value, then make sure the


How about if the newSchema is an optional schema or has a default value that is null, then make sure the NULL option is the first.

(optional_no_default || optional_null_default) != (optional || has_default)

wmoustafa · 2021-05-23T07:12:14Z

core/src/main/java/org/apache/iceberg/avro/PruneColumns.java

-    if (isOptionSchemaWithNonNullFirstOption(newSchema)) {
+    // if the newSchema is an optional schema with no, or null, default value, then make sure the
+    // NULL option is the first
+    boolean hasNonNullDefaultValue = field.hasDefaultValue() && AvroSchemaUtil.isNonNullDefault(field.defaultVal());


Refactoring as recommended above may help here.

wmoustafa · 2021-05-23T07:28:02Z

core/src/main/java/org/apache/iceberg/avro/SchemaToType.java

-      Object defaultValue = field.hasDefaultValue() && !(field.defaultVal() instanceof JsonProperties.Null) ?
-          field.defaultVal() : null;
+      Object defaultValue =
+          field.hasDefaultValue() && AvroSchemaUtil.isNonNullDefault(field.defaultVal()) ? field.defaultVal() : null;


What is the value of calling isNonNullDefault here? It just another way to do the same check as before !(field.defaultVal() instanceof JsonProperties.Null)?

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

api/src/main/java/org/apache/iceberg/types/Types.java

core/src/main/java/org/apache/iceberg/SchemaParser.java

core/src/test/java/org/apache/iceberg/TestSchemaParserForDefaultValues.java

core/src/main/java/org/apache/iceberg/SchemaParser.java

wmoustafa · 2021-06-11T05:11:49Z

core/src/main/java/org/apache/iceberg/SchemaParser.java

+
+  private static String toJsonString(Object value) {
+    if (isPrimitiveClass(value)) {
+      return value.toString();


For FIXED and BINARY, should do something like Base64.getEncoder().encodeToString(bytes)? (also the opposite transformation on the deserialization side).

good catch! on it

wrapped into ByteBuffer

core/src/main/java/org/apache/iceberg/SchemaParser.java

funcheetah

Thanks @shenodaguirguis ! This PR looks good to me in general and leave a few minor comments. One question I have: is default value for complex union supported?

api/src/main/java/org/apache/iceberg/types/Types.java

core/src/test/java/org/apache/iceberg/avro/TestAvroOptionsWithNonNullDefaults.java

…RO files

funcheetah

Thanks a lot @shenodaguirguis for this feature!

wmoustafa reviewed May 23, 2021

View reviewed changes

shenodaguirguis force-pushed the master branch from d0e58a6 to 82c7c55 Compare May 25, 2021 16:04

shenodaguirguis force-pushed the master branch from 82c7c55 to 3318285 Compare June 9, 2021 22:29

wmoustafa reviewed Jun 11, 2021

View reviewed changes

funcheetah reviewed Jun 11, 2021

View reviewed changes

api/src/main/java/org/apache/iceberg/types/Types.java Show resolved Hide resolved

core/src/test/java/org/apache/iceberg/avro/TestAvroOptionsWithNonNullDefaults.java Outdated Show resolved Hide resolved

shenodaguirguis force-pushed the master branch 2 times, most recently from c5de40a to 1a15a81 Compare June 16, 2021 19:26

[#2039] Support Default Value Semantis: use default values reading AV…

a1d338e

…RO files

shenodaguirguis force-pushed the master branch from 1a15a81 to a1d338e Compare June 16, 2021 19:55

funcheetah approved these changes Jun 17, 2021

View reviewed changes

wmoustafa approved these changes Jun 17, 2021

View reviewed changes

wmoustafa merged commit b81ab9e into linkedin:master Jun 17, 2021

shenodaguirguis mentioned this pull request Jun 18, 2021

Default Value Support Prototype (not for review) #70

Closed

shenodaguirguis mentioned this pull request Feb 3, 2022

[#2039] Support default value semantics - API changes apache/iceberg#2496

Closed

		Schema.Field newField = new Schema.Field(newFieldName, newFiledSchema, null, defaultValue);
		newField.addProp(AvroSchemaUtil.FIELD_ID_PROP, expectedField.fieldId());

Conversation

shenodaguirguis commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shenodaguirguis commented May 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

funcheetah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

funcheetah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shenodaguirguis commented May 15, 2021 •

edited

Loading